Skip to content

Fix function compute_scan_angle (QC flags), remove unused argument - #1780

Merged
BenjaminRuston merged 23 commits into
developfrom
bugfix/cd_compute_scan_angle
Aug 18, 2026
Merged

Fix function compute_scan_angle (QC flags), remove unused argument#1780
BenjaminRuston merged 23 commits into
developfrom
bugfix/cd_compute_scan_angle

Conversation

@chengdang

Copy link
Copy Markdown
Member

Description

This PR fixes compute_scan_angle by:

  • removing the unused argument
  • fixing QC-flag handling
  • updating all instrument converter scripts that call compute_scan_angle

Issue(s) addressed

Resolves issue #1775

Dependencies

None

Impact

None

Checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have run the unit tests before creating the PR

@chengdang chengdang self-assigned this Apr 30, 2026
@chengdang chengdang added the needs review Asking others to review - often used for pull requests label Apr 30, 2026
@chengdang

Copy link
Copy Markdown
Member Author

Have not tested it, needs to rebuild ioda

@BenjaminRuston

Copy link
Copy Markdown
Collaborator

needs to rebuild ioda

@chengdang actually for these you don't technically need to rebuild, just copy the new def_jedi_utils.py into the $JEDI_BUILD directory,, the path is in I think lib/python/....

thanks for getting this in and for giving it the attention it needed

@chengdang

Copy link
Copy Markdown
Member Author

@BenjaminRuston
Thanks! That's good to know. I was wondering if I need to re-build. Let me run some tests then.

@BenjaminRuston

Copy link
Copy Markdown
Collaborator

ok yes the hack is to obviously get on the branch and then you have to copy the def_jedi_utils.py file to:

${JEDI_BUILD}/lib/python3.11/pyiodaconv

so that's a shortcut to do a quick test

@chengdang

Copy link
Copy Markdown
Member Author

Thank you @BenjaminRuston
I did a test with the aws file, here is the post-ioda view angle:

image

I think it is working as expected, but more tests with other sensors are probably needed.

Comment thread src/pyiodaconv/def_jedi_utils.py Outdated
Comment thread src/pyiodaconv/def_jedi_utils.py Outdated
# do we need a missing here
ratio = np.empty_like(sensor_altitude)
# Initialize output with missing values
scanang = np.full_like(sensor_altitude, np.nan, dtype=float)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chengdang just one small change, this utility also tried to standardize the int, float and long missing values put into the files... they are globals up at the top

update to float missing from np.nan
@BenjaminRuston
BenjaminRuston marked this pull request as ready for review May 14, 2026 17:31
Comment thread src/pyiodaconv/def_jedi_utils.py Outdated
Comment thread src/pyiodaconv/def_jedi_utils.py Outdated
Comment thread src/pyiodaconv/def_jedi_utils.py Outdated
Comment thread src/pyiodaconv/def_jedi_utils.py Outdated
# do we need a missing here
ratio = np.empty_like(sensor_altitude)
# Initialize output with missing values
scanang = np.full_like(sensor_altitude, float_missing_value, dtype=float)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test are failing, so is the value supposed to be double missing value?

DIFFER : VARIABLE : sensorViewAngle : TYPE : DOUBLE <> FLOAT
DIFFER : TYPES : ATTRIBUTE : _FillValue : VARIABLE : sensorViewAngle : DOUBLE <> FLOAT

@BenjaminRuston

Copy link
Copy Markdown
Collaborator

ok @rajichidamb picked up your other changes and yes got some failures, switching to double to see if the failures change as this may fix those and cause other errors now elsewhere

think we have a situation where some converters used float and other double for the missing value

Comment thread src/pyiodaconv/def_jedi_utils.py Outdated
# do we need a missing here
ratio = np.empty_like(sensor_altitude)
# Initialize output with missing values
scanang = np.full_like(sensor_altitude, double_missing_value, dtype=float)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting dtype=float but using double_missing_value? Other python scripts fill the array with 'float32'

 sensor_altitude,
        np.array(f['GeolocationAndFlags']['earth_inc_ang'], dtype='float32'),

@BenjaminTJohnson

Copy link
Copy Markdown

The np.full_like(..., float_missing_value) initialization resolves the uninitialized-memory problem in the old np.empty_like path, and the new QC handling fixes the old if not qc_flag[0] bug that silently discarded the QC array whenever the first ob was good. A few things before this merges:

  1. Two call sites still use the old signature (silent breakage). src/hdf5/aws_2ioda.py (~L69) and src/satpy/irs_2ioda.py (~L310) still call with the old 3-positional form. Since the removed argument was first, Python won't error, the args just shift (zenith → altitude, altitude → zenith, zenith → qc_flag), so nearly all obs get flagged bad and sensorViewAngle comes out missing. These need updating in this PR (if there's not corresponding PRs?) Worth also double-checking cloudsat_netcdf_to_ioda.py, ozone_product_2ioda.py, wsfm_2ioda.py, and seviri_l1b_to_ioda.py, which import the function.

  2. Altitude units are inconsistent across callers. The function divides sensor_altitude by 1000 (expects meters), but amsr2 (strip('km')), gmi (407.0), and tropics (550.) pass km (maybe others??): for those, the computed scan angle collapses to ~ the zenith angle. Pre-existing, but since this PR fixes the function, could we document the expected unit in the docstring and audit callers (or handle it here)?

  3. Minor:

  • dtype=float makes the output float64; suggest np.float32 for consistency with ioda_float_type (or just be rigorous about using the correct float type that's already defined)?
  • double_missing_value is added but unused, was there a plan here?
  • Reference/CI data may need regenerating for converters that pass QC flags (cowvr especially), since outputs legitimately change.

Comment thread src/pyiodaconv/def_jedi_utils.py Outdated
@BenjaminRuston

Copy link
Copy Markdown
Collaborator

thanks for the re-review @BenjaminTJohnson , have made the changes and confirmed satellite altitudes for those calling this function at least are in meters

@BenjaminRuston
BenjaminRuston self-requested a review August 18, 2026 17:33
@rajichidamb

Copy link
Copy Markdown
Contributor

Reference files need update to match the new calculated sensorViewAngle

@BenjaminRuston BenjaminRuston removed the needs review Asking others to review - often used for pull requests label Aug 18, 2026

@BenjaminRuston BenjaminRuston left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this clean up @chengdang it definitely helps and caught some inconsistencies with definition of satellite altitude. In short it was very useful

@BenjaminRuston
BenjaminRuston merged commit 134c119 into develop Aug 18, 2026
2 checks passed
@BenjaminRuston
BenjaminRuston deleted the bugfix/cd_compute_scan_angle branch August 18, 2026 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants